Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added GlobalMessage to TripSearchScreen #4918

Merged
merged 3 commits into from
Jan 20, 2025

Conversation

strandlie
Copy link
Contributor

Closes AtB-AS/kundevendt#19786

There are no Figma sketches for this, but here is a screenshot:

Simulator Screenshot - iPhone 16 Pro - 2025-01-13 at 13 03 08

@@ -404,6 +405,18 @@ export const Dashboard_TripSearchScreen: React.FC<RootProps> = ({
{t(TripSearchTexts.searchState.noResultReason.MissingLocation)}
</ThemeText>
)}
{!!tripPatterns.length && (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be shown based on loading state instead of length? We might want to show messages in cases where there are no results. Or can it be shown even if loading hasn't completed?

Copy link
Contributor Author

@strandlie strandlie Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point. I thought tripPatterns.length was a reasonable proxy for loading state, but you are right that there are cases where that is not the case. Will fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, if I use !isSearching there is a delay after tripPatterns has elements which leads to a little uncomfortable experience. See the videos. The second video is using tripPatterns.length

isSearching.mp4
tripPatterns-length.mp4

Copy link
Member

@gorandalum gorandalum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments after pair-review:

  • Lets just show the box regardless of searching state, as suggested by Johannes
  • Use rule variables from the search results instead of search parameters, as using search parameters are a little fragile
  • Suggested rule variables: transportmode/transportsubmode and authority. Should be safe showing message for AtB shuttle buses

@strandlie strandlie force-pushed the strandlie/global-message-trip-results branch from db0d9f4 to 869d9a1 Compare January 14, 2025 14:45
@strandlie
Copy link
Contributor Author

strandlie commented Jan 14, 2025

👆🏽 Force push because of rebase on master

@@ -102,7 +102,7 @@ function transportModeToEnum(
transportMode: enumFromString(TransportMode, internal.transportMode),
transportSubModes: internal.transportSubModes
?.map((submode) => enumFromString(TransportSubmode, submode))
.filter(Boolean) as TransportSubmode[],
.filter(isDefined),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a cast I noticed that could be replaced with something safer :)

@strandlie strandlie merged commit c8141ba into master Jan 20, 2025
4 checks passed
@strandlie strandlie deleted the strandlie/global-message-trip-results branch January 20, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants